Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept smtp_settings #1

Merged
merged 1 commit into from
May 1, 2015
Merged

Conversation

janko
Copy link
Contributor

@janko janko commented Apr 30, 2015

I really like how thin the SimpleMailer is, and it made me realize how easy it is to actually use Net::STMP. I want to use it instead of Mail gem, because Mail gem is heavy, and I only need plain text emails for my application.

I was really missing SMTP settings configuration on SimpleMailer, I don't know how you actually send emails without it. So I added almost the same one as the Mail gem (without some needless method checking, I guess for Ruby 1.8.3).

I didn't really know how to test this properly, I don't know if it's necessary. I don't really know how to write RSpec tests to be compatible with all versions of RSpec, so I was thinking I could leave it to you :). I can successfully send an email with this commit.

@jeremyevans
Copy link
Owner

Thanks for the patch. The code looks fine, though I'm not sure it works if you don't assign a hash to smtp_settings.

The existing code has worked fine for me as none of the smtp servers I've used needed authentication, so the only thing I've needed to set is the SMTP server. I use this internally where I work, and I control both the client and server machines in all cases.

Could you please add specs, even if they just work on the current version of rspec? If they don't work for other rspec versions, I'll fix that. I will probably be switching the test suite to minitest/spec soon, so if you want to submit specs in that format, that's fine too.

@jeremyevans
Copy link
Owner

Also, note that about 70% of mail's memory usage is due to mime-types, and I've got a patch to mime-types that drastically reduces its memory usage, making it possible to use mail without a lot of overhead: mime-types/ruby-mime-types#96. The overhead is still higher than simple_mailer, but mail does a lot more.

@jeremyevans
Copy link
Owner

I've switched the test suite to minitest/spec, hopefully that make it easier to write the specs for this.

@janko
Copy link
Contributor Author

janko commented May 1, 2015

Updated. I changed it slighly that instead of an attribute writer for smtp_settings you just update the hash with default values. That also gives correct behaviour.

Regarding the Mail gem, the thing that I mind the most is the load time. To require Mail and to send a test email it takes me 0.5 seconds, which really stands out when I have a Roda application and want to care that my tests stay fast (and as I understood from the PR the load time hasn't changed). And it's a small project, I would probably go with Mail gem for something bigger.

@jeremyevans
Copy link
Owner

If the load time for Mail bothers you most, the patch to mime-types should drop it by about 2/3, this is mentioned in the first paragraph of the pull request.

I'll be merging and testing this later today, and assuming no problems I'll push it up to GitHub and release a new version of the gem. Thanks for your help!

@jeremyevans jeremyevans merged commit 301923d into jeremyevans:master May 1, 2015
@janko janko deleted the smtp-settings branch May 1, 2015 17:09
@janko
Copy link
Contributor Author

janko commented May 2, 2015

Unrelated, I'm curious, why did you switch SimpleMailer (and now Roda) to minitest/spec? I always figured that if someone wants to use the "assert" syntax, they should use Minitest, but if they want the "spec" syntax, then they're much better off with RSpec. I'm just curious to what are the advantages of using minitest/spec.

@jeremyevans
Copy link
Owner

minitest/spec is smaller and simpler, and fits my testing philosophy better. I've read most of the minitest codebase and it is just nicer to work in and easier to extend. I've already worked on a minitest extension that adds support for all of rspec hooks, including an around_all hook that rspec doesn't support without hacks using fibers. And with minitest you can use minitest_bisect to easily debug test order dependency bugs, which are usually a complete pain to debug.

There are a few nits I have with minitest, but I'm trying to get them addressed upstream, check the current pull requests.

I recommend watching this presentation from RailsConf 2015: http://confreaks.tv/videos/railsconf2015-ruby-on-rails-on-minitest. After watching it and talking with the presenter (and author of minitest), I decided to switch all of my libraries and apps to minitest/spec. I think the only library I have remaining is Sequel, which will take a bit more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants